Skip to content

gh-496: functions for legacy mode #497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 17 commits into from
Feb 18, 2025
Merged

gh-496: functions for legacy mode #497

merged 17 commits into from
Feb 18, 2025

Conversation

ntessore
Copy link
Collaborator

@ntessore ntessore commented Feb 1, 2025

Closes: #496

Implement functions and an example for FLASK-like "legacy mode". To be merged after #494. Example notebook not finished yet.

@ntessore ntessore force-pushed the nt/496 branch 4 times, most recently from bd57b91 to c1293de Compare February 3, 2025 10:41
@ntessore ntessore marked this pull request as ready for review February 3, 2025 23:10
@Saransh-cpp Saransh-cpp added api An (incompatible) API change blocked The issue or pull request is blocked by something labels Feb 4, 2025
@Saransh-cpp
Copy link
Member

Will review once #494 is in ✅

@ntessore ntessore mentioned this pull request Feb 4, 2025
Merged
@paddyroddy paddyroddy changed the base branch from main to nt/grf February 12, 2025 12:15
@paddyroddy
Copy link
Member

Will review once #494 is in ✅

Similarly, I shall wait. I have changed the head of this PR to be nt/grf.

@ntessore ntessore linked an issue Feb 13, 2025 that may be closed by this pull request
Create a bibliography in the docs #508
Closed
Base automatically changed from nt/grf to main February 13, 2025 15:37
ntessore
ntessore commented Feb 13, 2025
View reviewed changes
@ntessore ntessore removed the blocked The issue or pull request is blocked by something label Feb 13, 2025
@ntessore
Copy link
Collaborator Author

Looking at the coverage, I think a test went missing when I spliced this onto GLASS.

Unrelated to this PR, any ideas why all files and coverages appear twice on coveralls?

paddyroddy
paddyroddy reviewed Feb 14, 2025
View reviewed changes
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you send a link to the flask-like legacy mode?

@ntessore ntessore removed a link to an issue Feb 14, 2025
Create a bibliography in the docs #508
Closed
@ntessore ntessore mentioned this pull request Feb 14, 2025
Merged
Saransh-cpp
Saransh-cpp requested changes Feb 14, 2025
View reviewed changes
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ntessore! See my typing fixes below -

@Saransh-cpp Saransh-cpp mentioned this pull request Feb 14, 2025
Closed
@ntessore @Saransh-cpp
typing suggestions from code review
a500455 
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
@Saransh-cpp
Copy link
Member

pre-commit.ci autofix

Saransh-cpp
Saransh-cpp approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ntessore! Should wait for the discussion about linting above to be resolved.

@ntessore @paddyroddy
Update pyproject.toml
47db15b 
Co-authored-by: Patrick J. Roddy <patrickjamesroddy@gmail.com>
paddyroddy
paddyroddy approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Saransh-cpp
Copy link
Member

pre-commit.ci autofix

@ntessore
Copy link
Collaborator Author

I think this one is good to go. There's one untested function glass.fields.check_posdef_spectra() but I am not entirely sure what a good test for this would be.

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Feb 17, 2025 •
edited
Loading

but I am not entirely sure what a good test for this would be.

I think checking for two matrices, one that is posdef and one that is not, should be a good starter?

@ntessore
Copy link
Collaborator Author

but I am not entirely sure what a good test for this would be.

I think checking for two matrices, one that is posdef and one that is not, should be a good starter?

Yeah, maybe you are right, I was only thinking that is really testing that np.linalg works, not anything about our code. But close enough.

@ntessore ntessore merged commit 67a0af1 into main Feb 18, 2025
16 checks passed
@ntessore ntessore deleted the nt/496 branch February 18, 2025 13:51
@paddyroddy paddyroddy mentioned this pull request Mar 7, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paddyroddy paddyroddy paddyroddy approved these changes

@Saransh-cpp Saransh-cpp Saransh-cpp approved these changes

Assignees

@ntessore ntessore

Labels
api An (incompatible) API change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement "legacy mode"
3 participants
@ntessore @Saransh-cpp @paddyroddy